perf(regex): identify a literal by its source site, not by its pattern text - #9892
perf(regex): identify a literal by its source site, not by its pattern text#9892proggeramlug wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe PR adds compiler-emitted site keys for RegExp literals, a runtime site-key cache with weak compiled programs, configurable newborn barrier gating, diagnostic counters, and tests for cache identity, fallback behavior, GC state, and diagnostics. ChangesRegExp literal construction
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk was identified. Sequence Diagram(s)sequenceDiagram
participant Compiler
participant js_regexp_new_site
participant SiteKeyCache
participant RegExpHeader
participant GCBarrier
Compiler->>js_regexp_new_site: pass pattern, flags, site_key
js_regexp_new_site->>SiteKeyCache: lookup site_key and raw flags
SiteKeyCache-->>js_regexp_new_site: cached data or miss
js_regexp_new_site->>RegExpHeader: allocate and initialize header
RegExpHeader->>GCBarrier: check newborn parent state
GCBarrier-->>RegExpHeader: gate or execute field barriers
js_regexp_new_site->>SiteKeyCache: record entry or install programs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2cff89d to
61b4c21
Compare
…at cannot fail silently `PERRY_REGEX_DIAG` gains four per-construction work counters — `barrier_taken` / `barrier_gated` (whose sum must equal `new`), `header_bytes`, `site_verify_bytes` and `side_table_inserts` — so what `js_regexp_new` costs per call is a number rather than a reading of a profile. Writers for the first group arrive with the change they measure; `site_verify_bytes` is written here. `site_verify_bytes` is deliberately NOT `pattern_bytes`: the latter counts every construction's pattern length whether the site-cache probe hit or missed, while the full byte compare that verifies a fingerprint match is the part that is linear in the pattern — what makes a 12 KB emoji pattern expensive and a 60-byte one free. Counted at the construction probe only; `insert` and `install_programs` verify too and are not counted here. Two reliability fixes, both of the same shape as the campaign's missing exit-line trap — an absent output that greps identically to an instrument that was never built: * a file sink that cannot write now reports the path and the error on stderr once and keeps writing there, instead of swallowing the error; * the first snapshot is written at the first tick rather than one full DUMP_INTERVAL_MS later, so a run shorter than a second produces output.
Since PerryTS#9845 the `RegExpHeader` is a nursery allocation, so its two string field stores cannot owe the remembered set anything — and they were still taking the full barrier twice to discover that: four page-map classifications, two dirty-page-cache probes and two child classifications per construction, every one of them ending at `ParentNotOldSkips`. The gate is the runtime twin of the one the compiler already emits in front of every one of its own stores (`emit_parent_may_need_remembering_check`, PerryTS#7511): `GC_FLAG_TENURED` clear on the parent's LIVE header, and a globally idle incremental mark barrier. The first clause answers the generational question; the second is what makes it legal to skip the SATB/insertion shading as well, and dropping either one is a live child swept. Both are read live, so a header a collection promoted between `arena_alloc_gc` and the store, and `RegExp.prototype.compile` reassigning a tenured receiver, still take the full path. `gc::tests::inline_generation_gate_contract` already pins those two clauses for the emitted gate against a stranded-child witness; it now pins the runtime twin to the same codegen predicate clause by clause, and a third test asserts on the header `js_regexp_new` actually returns — so the skip arm is proven REACHED, not merely available. Measured motivation (segment-loop probe, region B, 60,000 reps, `sample`, main thread, leaf sum = thread header exactly): one `RegExp` per grapheme from a literal inside a function body, and the barrier subtree under `js_regexp_new` is 739 of 14,628 main-thread samples — 32 % of that function's own subtree. `PERRY_REGEX_NEWBORN_BARRIER_GATE=0` restores the unconditional pair. With the gate off nothing else changes, so the OFF arm is the pre-change code path exactly rather than a control still carrying the bookkeeping.
…n text `regex::site_cache` answers "have I seen this pattern TEXT before?" — the right question for a dynamic `new RegExp(s)`, and the wrong one for a literal, which is one source site whose pattern and flags are fixed at compile time. Because a content fingerprint can collide, every hit is verified by a full byte compare of the pattern, and a literal constructs a fresh object every time it is reached: on claude-code that verify is ~2.0 GB of `memcmp` per 400-character reply and 39.6 % of `js_regexp_new`'s own profile subtree. `Expr::RegExp` now emits an 8-byte private global per literal site and passes its ADDRESS to a new `js_regexp_new_site(pattern, flags, site_key)`. The address is unique by construction, immortal and never moves — the three properties a `StringHeader` address lacks, which is why the earlier analysis concluded no sound string identity was available and left the compare in place. A hit compares one word plus the site's <= 8-byte flags text and then reads nothing about the pattern: no fingerprint, no memcmp, no validation (validity is a pure function of the pair and the site's first construction established it), no flag canonicalization (the seven bits are a property of the site), and the programs the site already compiled are installed eagerly. `site_key = 0` is "no site": every dynamic construction keeps the two-argument entry and never touches the table. Kill switch `PERRY_REGEX_SITE_KEY=0`. Tests: two sites whose patterns have EQUAL LENGTH and different text, each constructed twice — the sabotage of keying the table by pattern length hands the second site the first's entry and fails on `.source` and on `test`; four dynamic constructions leave the table empty while one site-keyed construction fills it; a second construction at an executed site is born built; and the new symbol's declaration is asserted by name AND arity, because a missing declare fails only at the LLVM parse and a wrong arity miscompiles silently. Measurement is owed on the cc rig, where the 12,807-character pattern lives — the segment-loop probe's literal is ~60 characters and its memcmp is 0.16 % of the thread, so the probe cannot show this change.
…d counters moving the dump Two corrections from the I6 cc arm, both found by reading the instrument back rather than by argument. **1. The site table must never be the reason a program stays alive.** Measured on one 3300-char reply: settled footprint 478/474 MB -> 500/527 MB and idle CPU 2.37 -> 2.68 s against main. 1,024 entries at ~19 KB per compiled program is that order, and the campaign's directive is both metrics together — a CPU win bought with resident memory does not land. The entry now holds `Weak<Regex>` / `Weak<fancy_regex::Regex>` / `Weak<RepeatMatcherRegex>`; strong references stay where they belong, in the `(pattern, flags)` program caches and in every live header that installed them with `Arc::into_raw`. An entry whose programs have expired reports "not built yet" and the next construction re-picks them up from the content cache — the same path the site's first construction takes, so the lane self-heals. The upgrade is ALL-OR-NOTHING. PerryTS#9801 fixed an incoherent triple — a standard program memoized beside a missing fancy fallback — which does not error, it silently never matches; three independent `Arc` lifetimes reintroduce exactly that shape unless one dead reference invalidates the whole entry. Pinned by a test that drops ONLY the fancy program and asserts the entry reports unbuilt, which the natural per-field upgrade fails. **2. An added counter moved the instrument's own sampling.** `regex_with` counts every call as an event and dumps every `TICK_EVERY` events after a second has passed, so a second probe on an already-instrumented path doubles that path's event rate and moves the snapshot a SIGKILLed process leaves behind. On the I6 pair that showed up as `new / t` 206 k/s vs 173 k/s between two arms whose per-call ratios agree to 0.13 %, i.e. the two files describe different windows of the same workload. `regex_counters` accumulates without ticking the dump clock, and the three counters that ride along on already instrumented paths (barrier gate outcome, side-table inserts, site-verify bytes) now use it.
…S#9890 is fixed The comment at the `Expr::RegExp` lowering described the artifact-discarding bail-out in `codegen/method.rs` in the present tense. PerryTS#9896 fixed it: every return there now goes through `publish_lowered_fn_artifacts`, which drains all three collections and restores `llmod.ic_counter`, closing the duplicate site-id half as well. Rewritten as the obligation rather than the bug — every lowering exit must PUBLISH `typed_parse_rodata`, and a future early return that drops it breaks this site loudly at the in-process LLVM parse. A comment describing a hazard that no longer exists is a false lead, which is the thing it was written to prevent.
e760262 to
91a7791
Compare
… store - shape_descriptor_census asserted the dedicated GC birth kind inside `js_regexp_new`. #9892 split construction into a thin js_regexp_new / js_regexp_new_site pair over a shared js_regexp_new_impl, which is where the allocation now lives, so the census read a wrapper with no birth site. It follows the birth site instead. Verified the retargeted gate still fails when the kind is blunted inside js_regexp_new_impl. - gc_store_site_inventory wanted a marker on `REGEXP_PROTOTYPE_PTR.store`. The store is already correct — the sibling closure store spells out its barrier only because it uses with_slot and bypasses the wrapper, while RealmAtomicI64::store routes through runtime_store_root_atomic_raw_i64 itself. Marker records that.
|
Landed on |
Depends on #9891 (stacked; the diff shown here includes it until that lands).
Stacked on
perf/regex-newborn-barrier-gate.What
regex::site_cacheanswers "have I seen this pattern TEXT before?" — theright question for a dynamic
new RegExp(s), and the wrong one for a literal,which is one source site whose pattern and flags are fixed at compile time.
Because a content fingerprint can collide, every hit is verified by a full
byte compare of the pattern (
site_cache.rs:125,&*entry.pattern == pattern). A literal evaluates to a fresh object every timeit is reached, and claude-code's segment loop constructs
string-width's~12,807-character
/…/gonce per grapheme:PERRY_REGEX_DIAGmeasured 2.0 GBof
memcmpper 400-character reply, and asampleput_platform_memcmpat39.6 % of
js_regexp_new's own subtree.Expr::RegExpnow emits an 8-byte private global per literal site(
@perry_regexp_site_<prefix>__<n>,private global i64 0) and passes itsaddress to a new
js_regexp_new_site(pattern, flags, site_key).Why an address and not an integer id
An integer id would have to be unique across separately compiled modules. An
address is unique by construction (distinct globals have distinct addresses),
immortal and never moves — the three properties a
StringHeaderaddresslacks, which is exactly why the earlier analysis of this problem concluded no
sound string identity was available and left the byte compare in place: string
headers are GC-managed, so an address is freed and reused, and a moving
collector relocates them, and a pointer-keyed cache over them answers for a
different pattern.
A hit therefore verifies with one word plus the site's ≤ 8-byte raw flags
text — flags are compared because two spellings of one canonical form (
/x/ig,/x/gi) must not answer for each other — and then reads nothing about thepattern: no fingerprint, no
memcmp, no validation (validity is a pure functionof the pair and the site's first construction established it), no flag
canonicalization (the seven bits are a property of the site). Once the site's
first header has executed, later constructions are born built.
site_key = 0means "no site": every dynamic construction(
js_regexp_construct,RegExp.prototype.compile, the runtime's own callers)keeps the two-argument entry point and never touches the table.
Tests
sites, same flags, same pattern length, different text, each constructed
twice (a first construction always misses and would pass under every
sabotage). Under a length- or prefix-keyed table the second site inherits the
first's entry:
.sourcereports a pattern the literal never contained andtestmatches the wrong language.js_regexp_newcalls leavethe table empty; one
js_regexp_new_sitecall fills it — so the zero is aproperty of the entry point, not of a table that never works.
skipped the content cache would push the pattern's hash back onto the first
test()and the site key would buy nothing.declareisinvisible to every HIR-level test and fails only at the in-process LLVM parse
(feat(codegen): match the Intl.Segmenter for-of and answer it from the runtime view mode (default OFF) #9859's five segment-view externs, after twelve passing unit tests); a wrong
arity parses and miscompiles.
Kill switch
PERRY_REGEX_SITE_KEY=0— the probe misses and nothing is recorded, so the OFFarm is the content-keyed path exactly.
What the probe can and cannot show
The segment-loop probe cannot show this change. Its literal is ~60
characters, so its
memcmpis 0.16 % of the thread. The evidence has to comefrom the cc rig, where the 12,807-character pattern lives — a full bundle
compile with
PERRY_REGEX_DIAGarmed on one 3300-char reply.Registered prediction, as a ratio because the two captures are different
runs with different diag windows and only same-run counts may be registered as
absolutes:
site_verify_bytes / newcollapses toward zero whilecompiles std/fancy/repeatare unchanged. A compile count that movesmeans the site table answered for a pattern it should not have.
Retention caveat, since this adds state
The site table is 1,024 entries. Holding their programs strongly cost
+20…+50 MB settled and idle CPU 2.37 → 2.68 s on cc — measured, not
assumed, and exactly the trade the directive rejects. Fixed before this PR
went up for review: the entry holds them
Weak, so the table can handprograms out but can never be why they stay alive. See the Results section
below for the three-arm comparison that establishes it. What the entry still
holds strongly is the
(pattern, flags)text, which is small — but if a futurecapture shows settled memory above main's, that text is the next thing to bound
(cc's emoji literal is 12,807 bytes and there are 1,024 slots).
A pre-existing hazard this work surfaced
Filed as #9890 and since fixed by #9896:
codegen/method.rs's "parent classhas no callable constructor symbol" bail-out lowered the body and then discarded
ic_globals,typed_parse_rodataandpending_declares, so anything that bodydeclared was referenced by emitted IR and never defined. Pre-existing; the site
global this PR adds would have been one more victim. Every return there now goes
through
publish_lowered_fn_artifacts, which drains all three and restoresllmod.ic_counter— closing the quieter half too (site ids reissued to the nextfunction, giving a duplicate-symbol redefinition). The comment at the lowering is
now written as the standing obligation rather than as a live bug.
Results on cc (perrymaster, quiet box)
Three arms: main5, I6 (main5 + this stack, programs held strongly) and
I6b (I6 + the weak-programs fix,
59cc2a3fb, 6,116 site globals in thebundle). Raw:
/root/armI6B_9838.log,/root/rig9831/regexdiag_{I6b,I6,main5}_3300.txt,combI6B.jsonl.CPU — 5×3300, paired
MIN −6.4 %, mean −5.4 %, 5 of 5 paired draws. 400-char: 0.96 → 0.96
(unchanged). Under contention MIN is the estimator; both are reported because
the arms are load-matched here (load 4.9 → 0.5).
Counters — one 3300-char reply,
PERRY_REGEX_DIAGcompiles std/fancy/repeatsite_key_hit / newsite_verify_bytesbarrier_gated + barrier_takennewside_table_inserts / newheader_bytesThe compile counts are identical across all three arms — the registered
falsifier, which a site table answering for the wrong pattern would break.
Memory — the cost, stated plainly
Peak RSS at 3300: 608–616 → 618–626 MB, +3…+15 MB (~2 %). That is the
price of this change and it is not hidden: the directive is both metrics
together, so a reviewer should weigh ~2 % peak against −5…−6 % CPU rather than
read the CPU line alone.
Settled at 120 s: main5 480/477 vs I6b 488/489 MB (+8…+12). Read that as
not resolved at n=2, not as a win: main5's own settled figure ranged
474–510 MB across today's runs, a spread of 36 MB, which is wider than the
delta. What the third arm does establish is the direction of the fix — I6, with
the site table holding its programs strongly, settled at 500/527 (+20…+50)
and idle CPU 2.37 → 2.68 s. Holding them weakly removes most of that, which
confirms the strong program references were the retention rather than leaving
it to be argued.
400-char settled: 459/461 → 467/462.
Landing order and state
#9891 → #9892. This PR is stacked on the newborn-barrier gate; its diff
includes that PR until it lands.
Rebased onto
504e180d0. The only file main touched that this branch alsotouches is
runtime_decls/strings.rs, and the overlap is purely additive at adifferent site (
2a71d706edeclared the fivejs_segments_view_*externs atline 1584; this adds
js_regexp_new_siteat 1321). Neither side changed a typeor a contract the other depends on.
The rebase also rewrites one comment: the note at the
Expr::RegExploweringdescribed
codegen/method.rs's artifact-discarding bail-out in the presenttense, and #9896 fixed it (every return there now goes through
publish_lowered_fn_artifacts, which drains all three collections and restoresllmod.ic_counter, closing the duplicate site-id half too). It is now writtenas the standing obligation — every lowering exit must publish
typed_parse_rodata— because a comment describing a hazard that no longerexists is the false lead it was written to prevent.
Green before the rebase (
e76026209): runtime lib 3,246 passed / 0 failed /4 ignored (exactly +2 against the previous SHA's 3,244 — the two
weak-programs tests),
perry-codegenRC=0, gate-equivalent clippy rc=0 with nowarning naming any file this stack touches. The rebased tree is type-checked
(
cargo check --release --all-targets, lib and tests, rc=0 for bothperry-runtimeandperry-codegen, with no warning naming a file this stack touches); the dev box is at 8 GB free,below the campaign's 12 GB build floor, so the full suite has not been re-run on
the rebased SHA and CI is the compile gate for it.
The changelog fragment's numeric prefix is
9886, not this PR's number, and#9891's is
9885. The gate hard-fails only on a missing prefix and warns ona wrong one deliberately —
scripts/check_changeset_fragment.shsays a strictrule would block backfills and stacked PRs, which is exactly this pair.
Renumbering would mean rewriting both commits again immediately before landing,
for a warning the gate is designed to emit.
Summary by CodeRabbit
Performance
Compatibility
Diagnostics